fix: reduce redundant updates for handlers and gestures#2188
fix: reduce redundant updates for handlers and gestures#2188Yradex merged 6 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 0ec04e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR optimizes React runtime gesture handling by converting gesture Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 482ffeaefc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
482ffea to
77dec4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react/runtime/src/gesture/processGestureBagkround.ts`:
- Around line 45-51: The committedCallbacks cloning assumes every callback is
non-null and calls prepareWorkletForCommit(...)! which will throw when a
callback is null/undefined; update the loop over committedCallbacks (the object
derived from baseGesture.callbacks) to check each committedCallbacks[name] for
null/undefined and only call prepareWorkletForCommit when it is present,
otherwise leave the entry as-is (remove the non-null assertions and skip calling
prepareWorkletForCommit for null/undefined values) so null callbacks are
preserved and not spread into worklets.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
77dec4b to
b09eaa9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx`:
- Around line 142-199: The test reads lynx.getNativeApp().callLepusMethod too
soon after the background render, causing a race; after render(<Comp tick={1}
/>, __root) in the "rerender with no semantic changes" block, await the
scheduled patch (e.g., await Promise.resolve() or flush pending
timers/microtasks) before switching back to main thread and accessing
lynx.getNativeApp().callLepusMethod.mock.calls so that
getSnapshotPatchFromPatchUpdateCall sees the finalized call.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx (1)
12-16: Make helper failures explicit instead of a TypeError.
Ifcallorcall[1].datais missing, the current code throws a vague error. Adding explicit assertions makes failures easier to diagnose.Suggested change
function getSnapshotPatchFromPatchUpdateCall(call) { - const obj = call[1]; - const parsed = JSON.parse(obj.data); + expect(call, 'expected a patch update call').toBeTruthy(); + const obj = call[1]; + expect(obj?.data, 'expected patch payload').toBeTypeOf('string'); + const parsed = JSON.parse(obj.data); return parsed.patchList?.[0]?.snapshotPatch; }
Web Explorer#9079 Bundle Size — 899.89KiB (+0.13%).0ec04e7(current) vs ea5e30e main#9077(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/mts-support-use-callba... Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx`:
- Around line 26-34: The test overrides a global value but never restores it;
capture the original SystemInfo.lynxSdkVersion in the beforeEach (e.g., const
originalLynxSdkVersion = SystemInfo.lynxSdkVersion) before setting
SystemInfo.lynxSdkVersion = '999.999', and then restore it in afterEach (set
SystemInfo.lynxSdkVersion = originalLynxSdkVersion) alongside
vi.restoreAllMocks() and elementTree.clear() to prevent cross-test leakage.
- Around line 20-24: Tests mutate global state via injectUpdateMainThread
(assigning globalThis['rLynxChange']) and patch Preact's options.commit via
replaceCommitHook/hook(), so add an afterAll() that deletes
globalThis['rLynxChange'] and restores options.commit to its original value (use
the original reference captured by hook() or reset options.commit to
undefined/default) in the mtf-execid-churn.test.jsx file so test globals are
cleaned up in addition to vi.restoreAllMocks().
Merging this PR will degrade performance by 19.13%
Performance Changes
Comparing Footnotes
|
|
This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump"). |
23f46ef to
ab35b6c
Compare
React Example#7506 Bundle Size — 225.23KiB (+0.36%).0ec04e7(current) vs ea5e30e main#7504(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/mts-support-use-callba... Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#639 Bundle Size — 196.39KiB (+0.42%).0ec04e7(current) vs ea5e30e main#637(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/mts-support-use-callba... Project dashboard Generated by RelativeCI Documentation Report issue |
React External#624 Bundle Size — 679.93KiB (+0.76%).0ec04e7(current) vs ea5e30e main#622(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/mts-support-use-callba... Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0675b33c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx (2)
166-172: Nit:toJSONdestructured binding is unused.Minor readability nit — rename to
_to signal intent (the goal is only to striptoJSONfromrest).Proposed tweak
- toJSON() { - const { toJSON, ...rest } = this; + toJSON() { + const { toJSON: _toJSON, ...rest } = this; return { ...rest, __isSerialized: true, }; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx` around lines 166 - 172, The destructured binding named toJSON inside the toJSON method is unused and hurts readability; change the destructuring from `{ toJSON, ...rest } = this` to use a throwaway identifier (e.g. `{ _: _, ...rest } = this` or simply `{ _: , ...rest } = this` depending on style) so it's obvious the intent is only to remove the toJSON property and keep the remaining fields; update the toJSON method in the test (the toJSON method on the test object/class) to destructure the property into a clearly unused name like `_` and return `{ ...rest, __isSerialized: true }`.
49-215: Optional: extract the shared render/hydrate/rerender harness.The three
itblocks share near-identical main-thread-render → background-render → hydrate → rerender scaffolding; only the component and the assertion target differ. Extracting a helper likerunStableRerenderCase(Comp, triggerRerender)would cut ~60 lines and make future churn tests a one-liner. Deferable — current form is readable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx` around lines 49 - 215, Tests duplicate the same main-thread render → background render → hydrate → rerender scaffold across three it blocks; extract a helper (e.g., runStableRerenderCase) to remove duplication. Implement runStableRerenderCase(Comp, triggerRerender) that performs: set __root.__jsx and renderPage() for main-thread render; switch to background and call render(<Comp />, __root); run hydrate by invoking lynxCoreInject.tt.OnLifecycleEvent(...__OnLifecycleEvent.mock.calls[0]) then switch to main thread and invoke the recorded callLepusMethod; for rerender switch to background, clear mocks (lynx.getNativeApp().callLepusMethod.mockClear()), call triggerRerender (this can be bump_ or rendering with new props), await waitSchedule(), switch to main thread and return the rLynxChange so callers can assert getSnapshotPatchFromPatchUpdateCall(rLynxChange) is undefined; update the three tests to call runStableRerenderCase with their Comp and appropriate trigger function.packages/react/runtime/__test__/snapshot/gesture.test.jsx (1)
1148-1155: Consider also covering user-providedgestureattr preservation.This regression covers retaining
clip-radius/flatten, but the PR objective also calls out not clobbering unrelated user-providedgestureattrs. A small variant with bothgestureandmain-thread:gesture, then removing onlymain-thread:gesture, would lock that behavior down too.Also applies to: 1196-1199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/gesture.test.jsx` around lines 1148 - 1155, Add a test variant that ensures user-provided 'gesture' attributes are not clobbered when only 'main-thread:gesture' is removed: update the existing props construction (the variable props used with keepGesture and _gesture) to include a case where both 'gesture' and 'main-thread:gesture' are present, then simulate removal of only 'main-thread:gesture' and assert that 'gesture' remains unchanged; mirror the same change for the other related case referenced (the analogous code around the other instance of props). Ensure assertions reference the same props/_gesture symbols so the test verifies preservation of user-provided 'gesture' attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/__test__/snapshot/gesture.test.jsx`:
- Around line 1148-1155: Add a test variant that ensures user-provided 'gesture'
attributes are not clobbered when only 'main-thread:gesture' is removed: update
the existing props construction (the variable props used with keepGesture and
_gesture) to include a case where both 'gesture' and 'main-thread:gesture' are
present, then simulate removal of only 'main-thread:gesture' and assert that
'gesture' remains unchanged; mirror the same change for the other related case
referenced (the analogous code around the other instance of props). Ensure
assertions reference the same props/_gesture symbols so the test verifies
preservation of user-provided 'gesture' attributes.
In `@packages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsx`:
- Around line 166-172: The destructured binding named toJSON inside the toJSON
method is unused and hurts readability; change the destructuring from `{ toJSON,
...rest } = this` to use a throwaway identifier (e.g. `{ _: _, ...rest } = this`
or simply `{ _: , ...rest } = this` depending on style) so it's obvious the
intent is only to remove the toJSON property and keep the remaining fields;
update the toJSON method in the test (the toJSON method on the test
object/class) to destructure the property into a clearly unused name like `_`
and return `{ ...rest, __isSerialized: true }`.
- Around line 49-215: Tests duplicate the same main-thread render → background
render → hydrate → rerender scaffold across three it blocks; extract a helper
(e.g., runStableRerenderCase) to remove duplication. Implement
runStableRerenderCase(Comp, triggerRerender) that performs: set __root.__jsx and
renderPage() for main-thread render; switch to background and call render(<Comp
/>, __root); run hydrate by invoking
lynxCoreInject.tt.OnLifecycleEvent(...__OnLifecycleEvent.mock.calls[0]) then
switch to main thread and invoke the recorded callLepusMethod; for rerender
switch to background, clear mocks
(lynx.getNativeApp().callLepusMethod.mockClear()), call triggerRerender (this
can be bump_ or rendering with new props), await waitSchedule(), switch to main
thread and return the rLynxChange so callers can assert
getSnapshotPatchFromPatchUpdateCall(rLynxChange) is undefined; update the three
tests to call runStableRerenderCase with their Comp and appropriate trigger
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee8db453-f966-4ca7-b71e-2349b5aa4d0b
📒 Files selected for processing (9)
.changeset/fix-react-runtime-execid-churn.mdpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/composition.tspackages/react/runtime/__test__/gesture/prepareGestureForCommit.test.jspackages/react/runtime/__test__/gesture/processGesture.test.tspackages/react/runtime/__test__/snapshot/gesture.test.jsxpackages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsxpackages/react/runtime/src/gesture/processGesture.tspackages/react/runtime/src/gesture/processGestureBagkround.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/fix-react-runtime-execid-churn.md
- packages/lynx/gesture-runtime/src/baseGesture.ts
- packages/react/runtime/test/gesture/prepareGestureForCommit.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/lynx/gesture-runtime/src/composition.ts
- packages/react/runtime/test/gesture/processGesture.test.ts
- packages/react/runtime/src/gesture/processGestureBagkround.ts
- packages/react/runtime/src/gesture/processGesture.ts
- Implement copy-on-commit for worklets, gestures, and spread props to avoid background-side mutation. - Prevent _execId churn for stable references, reducing redundant native patches. - Fix gesture removal cleanup when removed from spread props. - Add regression tests for execId churn and gesture cleanup. - Add changeset for @lynx-js/react-runtime.
…assertions and awaiting schedule.
|
LGTM |
6aa232b to
0ec04e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/runtime/__test__/snapshot/gesture.test.jsx (1)
1215-1221: Optional: fixturetoJSONpatterns are inconsistent across this file.Most updated fixtures now use
toJSON: function() { return { ...this, __isSerialized: true }; }, but the two test-scopedcreateGesturehelpers (Lines 1215-1221, 1309-1315) still striptoJSONviaconst { toJSON, ...rest } = this. Both work, but the mixed style makes it harder to scan; consider unifying to one pattern. Purely cosmetic.Also applies to: 1309-1315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/gesture.test.jsx` around lines 1215 - 1221, Update the test-scoped createGesture helpers so their toJSON implementations use the unified pattern used elsewhere: replace the method that destructures off toJSON (currently written as toJSON() { const { toJSON, ...rest } = this; return { ...rest, __isSerialized: true }; }) with the concise form toJSON: function() { return { ...this, __isSerialized: true }; } in both createGesture helpers (the helper that defines toJSON at the top and the second test-scoped createGesture), ensuring both fixtures use the same style for consistency.packages/react/runtime/src/snapshot/gesture/processGestureBagkround.ts (1)
49-56: Optional: document the call-site invariant onattachCommittedSerializer.
Object.assign(gesture, { serialize, toJSON })mutates the input. Current callers (Lines 72 and 93) always pass a freshly spread copy, so this is safe — but nothing in the type signature enforces it, and a future caller could accidentally pass the original and leak the committed serializer onto the background-cached gesture, reintroducing the very mutation churn this PR is trying to eliminate. A one-line comment like// callers MUST pass a fresh copy; this mutates the argumentwould harden the contract without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/gesture/processGestureBagkround.ts` around lines 49 - 56, Add a one-line invariant comment above attachCommittedSerializer documenting that it mutates its argument and therefore callers MUST pass a fresh/shallow-copied gesture; reference attachCommittedSerializer and serializeCommittedGesture in the comment to make it clear this function assigns serialize/toJSON onto the passed-in gesture and that callers must not pass shared background-cached gesture objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/__test__/snapshot/gesture.test.jsx`:
- Around line 1215-1221: Update the test-scoped createGesture helpers so their
toJSON implementations use the unified pattern used elsewhere: replace the
method that destructures off toJSON (currently written as toJSON() { const {
toJSON, ...rest } = this; return { ...rest, __isSerialized: true }; }) with the
concise form toJSON: function() { return { ...this, __isSerialized: true }; } in
both createGesture helpers (the helper that defines toJSON at the top and the
second test-scoped createGesture), ensuring both fixtures use the same style for
consistency.
In `@packages/react/runtime/src/snapshot/gesture/processGestureBagkround.ts`:
- Around line 49-56: Add a one-line invariant comment above
attachCommittedSerializer documenting that it mutates its argument and therefore
callers MUST pass a fresh/shallow-copied gesture; reference
attachCommittedSerializer and serializeCommittedGesture in the comment to make
it clear this function assigns serialize/toJSON onto the passed-in gesture and
that callers must not pass shared background-cached gesture objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a210967-c23f-4248-adba-388abc9f41a0
📒 Files selected for processing (9)
.changeset/fix-react-runtime-execid-churn.mdpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/composition.tspackages/react/runtime/__test__/snapshot/gesture.test.jsxpackages/react/runtime/__test__/snapshot/gesture/prepareGestureForCommit.test.jspackages/react/runtime/__test__/snapshot/gesture/processGesture.test.tspackages/react/runtime/__test__/snapshot/mtf-execid-churn.test.jsxpackages/react/runtime/src/snapshot/gesture/processGesture.tspackages/react/runtime/src/snapshot/gesture/processGestureBagkround.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-react-runtime-execid-churn.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lynx/gesture-runtime/src/composition.ts
- packages/lynx/gesture-runtime/src/baseGesture.ts
- packages/react/runtime/test/snapshot/mtf-execid-churn.test.jsx
Summary by CodeRabbit
Summary
This PR reduces redundant native updates for stable main-thread handlers and gestures by separating cached background-side objects from the payloads committed to the main thread.
The previous flow mixed commit-time runtime metadata (for example
_execId) into objects that were also reused for diffing. As a result, a handler or gesture could stay semantically unchanged across rerenders but still look different to the patching layer, which caused unnecessary native updates. This PR moves those paths to copy-on-commit, and also tightens the spread-driven gesture removal flow so cleanup only touches ReactLynx-owned gesture state.Key points
1. Copy-on-commit for gestures and worklets
The main issue was that commit-time preparation mutated objects that were later reused for diffing. For stable references, that meant the diff saw runtime-only churn instead of a real semantic change.
Before, commit-time preparation effectively behaved like this:
That mutates the cached gesture object itself.
Now the runtime prepares a committed copy instead:
This keeps the background-side cached object stable while still attaching
_execIdto the payload sent to the main thread.2. Spread props only clone when commit-time rewriting is actually needed
Stable rerenders through spread props were still expensive because every changed spread was shallow-cloned and scanned, even when it only contained primitive attrs.
The old shape was effectively:
Now the spread commit path clones lazily:
That means ordinary spread updates keep the original object and avoid unnecessary allocation/scanning, while worklets and gestures still get rewritten correctly when needed.
3. Gesture removal from spread props now cleans up the right state
When a spread removed
main-thread:gesture, the removal happened late inupdateSpread(), after surviving keys had already been processed. That made cleanup order important.A problematic transition looked like this:
clip-radiusstill requiresflatten={false}, so clearingflattenduring gesture cleanup was incorrect. This PR fixes that by only clearing ReactLynx-owned gesture state and leaving unrelated spread state alone.In addition, when
__RemoveGestureDetectoris available, the runtime no longer forcefully writesgesture = nullitself. That avoids clobbering a user-providedgestureattr if detector cleanup is already owned by the native remove API.4. Committed gesture serialization no longer depends on mutating the original object
The gesture commit path now serializes the committed copy instead of relying on the original object's
toJSONbehavior. This makes the runtime more robust when callbacks have been rewritten for commit-time transport and avoids reintroducing_execIdchurn through serialization.Conceptually, the committed payload now comes from the committed object itself:
This keeps serialization aligned with the object that actually contains the committed callbacks.
5. Behavioral boundary of this change
This PR is intentionally scoped to runtime-owned commit and cleanup behavior:
main-thread:gesturefrom spread props should not clear unrelated state such asflatten.__RemoveGestureDetector, ReactLynx should not additionally clobber a user-providedgestureattr.Checklist